fix(UI): update user_id to username in ssh/token table#1330
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the user identification system by replacing user_id with username throughout the codebase, particularly in SSH key and token management tables. The change affects database schema, API endpoints, and authentication logic to use usernames as identifiers instead of user IDs.
- Updates SSH key and access token database tables to use username instead of user_id
- Refactors HTTP authentication logic by moving it from a standalone function to the SmartProtocol struct
- Adds username field to merge request model and updates related functionality
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| jupiter/src/storage/user_storage.rs | Updates all user storage methods to use username parameter instead of user_id |
| mono/src/api/user/user_router.rs | Changes API endpoints to use user.username instead of user.campsite_user_id |
| jupiter/callisto/src/ssh_keys.rs | Updates SSH keys model to use username field instead of user_id |
| jupiter/callisto/src/access_token.rs | Updates access token model to use username field instead of user_id |
| jupiter/src/migration/m20250812_022434_alter_mega_mr.rs | Adds database migration to rename user_id columns to username and add username to mega_mr table |
| ceres/src/protocol/mod.rs | Moves HTTP authentication logic to SmartProtocol and adds username field |
| mono/src/git_protocol/http.rs | Removes standalone http_auth function and updates to use protocol method |
| .ok_or_else(|| MegaError::with_message("Invalid key format"))? | ||
| .to_string() | ||
| } else { | ||
| json.title |
There was a problem hiding this comment.
The logic for determining the title has inconsistent conditional structure. The condition json.title.is_empty() is checked, but the original logic checked !json.title.is_empty(). This reversal changes the behavior - now when title is empty, it uses ssh_parts, and when title is not empty, it uses json.title. This appears to be the intended fix, but the logic flow could be clearer.
| )) | ||
| .await?; | ||
| } | ||
| DatabaseBackend::Sqlite => {} |
There was a problem hiding this comment.
The migration skips SQLite implementation entirely. This creates inconsistent behavior across database backends and could cause issues for developers using SQLite in development or testing environments.
| DatabaseBackend::Sqlite => {} | |
| DatabaseBackend::Sqlite => { | |
| // Add 'username' column to mega_mr table if it does not exist | |
| manager | |
| .get_connection() | |
| .execute(Statement::from_string( | |
| manager.get_database_backend(), | |
| "ALTER TABLE mega_mr ADD COLUMN username TEXT NOT NULL DEFAULT ''".to_owned(), | |
| )) | |
| .await?; | |
| // Rename 'user_id' to 'username' in access_token table | |
| manager | |
| .get_connection() | |
| .execute(Statement::from_string( | |
| manager.get_database_backend(), | |
| "ALTER TABLE access_token RENAME COLUMN user_id TO username".to_owned(), | |
| )) | |
| .await?; | |
| // Rename 'user_id' to 'username' in ssh_keys table | |
| manager | |
| .get_connection() | |
| .execute(Statement::from_string( | |
| manager.get_database_backend(), | |
| "ALTER TABLE ssh_keys RENAME COLUMN user_id TO username".to_owned(), | |
| )) | |
| .await?; | |
| } |
| } | ||
|
|
||
| pub fn username(&self) -> String { | ||
| self.username.clone().unwrap_or(String::from("Admin")) |
There was a problem hiding this comment.
Using a hardcoded fallback username 'Admin' could lead to confusion in logs and audit trails. Consider using a more descriptive default like 'system' or 'unknown', or better yet, make username required to avoid ambiguity.
| self.username.clone().unwrap_or(String::from("Admin")) | |
| self.username.clone().unwrap_or(String::from("system")) |
| pub async fn http_auth(&mut self, header: &HeaderMap<HeaderValue>) -> bool { | ||
| for (k, v) in header { | ||
| if k == http::header::AUTHORIZATION { | ||
| let decoded = general_purpose::STANDARD |
There was a problem hiding this comment.
The base64 decoding and string parsing operations use .unwrap() which will panic on malformed input. This creates a security vulnerability where malformed Authorization headers can crash the service.
| tracing::info!("repeat commit with mr: {}, do nothing", mr.id); | ||
| } | ||
| } else { | ||
| // TODO 直接覆盖？ |
There was a problem hiding this comment.
The comment is in Chinese and appears to be a TODO item asking about direct overwriting. Comments should be in English for consistency and the TODO should be clarified or addressed.
| // TODO 直接覆盖？ | |
| // TODO: Should we directly overwrite the existing MR in case of a hash conflict? Review and implement appropriate logic if needed. |
No description provided.